fix(cli): Improve number parsing #2454
Conversation
sirtimid
force-pushed
the
sirtimid/cli-parse-number
branch
from
6efdc03
to
26a8fd4
Compare
packages/cli/src/number-parse.js
Outdated
| if (typeof input === 'number' && !Number.isNaN(input)) { | ||
| return input; | ||
| } | ||
|
|
||
| if (typeof input === 'string' && /^-?\d+(\.\d+)?$/.test(input.trim())) { | ||
| return Number(input); | ||
| } |
There was a problem hiding this comment.
A thing you can rely on is that Number(string) will return NaN if the string doesn’t parse as any valid JavaScript representation of a number.
You can also assume parseNumber receives string and will never see a number as input, given that this is not a general purpose utility but specifically for parsing numbers for the CLI.
There was a problem hiding this comment.
(Notably, this will expand the range of expressible numbers to include 1e3 without requiring any additional complexity in a regex.)
There was a problem hiding this comment.
An alternative suggestion that I especially like for CLIs, from #2204:
Line 110 in 20b4d24
example valid inputs:
- "42"
- "-40.0"
- ".5"
- "1.337e3"
- "␣+1␣" (where "␣" is a visible representation of whitespace)
- "0B111" (binary for
7) - "0o040" (octal for
32) - "0xf00" (hexadecimal for
3840)
example invalid inputs (i.e., they return NaN):
- "f00"
- "NaN"
- "Infinity"
- "7up"
- " "
- "␣"
- (empty string)
undefined
There was a problem hiding this comment.
Thanks! I also added more test cases based on @gibson042 examples. @kriskowal What is weird is that Number('') returns 0 instead of NaN
sirtimid
force-pushed
the
sirtimid/cli-parse-number
branch
from
38c2a57
to
3b4a975
Compare
sirtimid
force-pushed
the
sirtimid/cli-parse-number
branch
from
3b4a975
to
4c026c5
Compare
Description
This PR introduces a new
parseNumberfunction that safely parses input as a number on cli commands.Additional tests have been added to ensure this new functionality works correctly.
Security Considerations
none
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
none
Compatibility Considerations
none
Upgrade Considerations
none